-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add explicit_struct_names
Extension
#522
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added some questions and comments
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #522 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 82 83 +1
Lines 14410 14443 +33
=========================================
+ Hits 14410 14443 +33 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the changes all look good - thank you for also adding documentation!
One question that we should still resolve is how this extension relates to the PrettyConfig::struct_names
option. I think it would make sense that enabling the extension during serialisation forces all struct names to be emitted (even when we're not in pretty serialising mode). Do we then still need the PrettyConfig::struct_names
option? I think if we can still emit struct names with and without (as two distinct options) emitting the explicit #[enable(explicit_struct_names)]
marker, we can probably also axe the config option.
I think it would be more intuitive to keep the serialization configuration options separate from the deserialization extension. If |
Co-authored-by: Juniper Tyree <[email protected]>
… compiler warning
It turns out |
So far, all extensions have served a dual purpose of specifying a variation of the RON format and enabling it during both serialisation and deserialisation, so that you can produce a RON document with the same options (that’s how the Options struct originated) that you will apply during parsing. Therefore, I think it would be good for consistency to keep that dual functionality. So far, explicit struct names have just been a prettifying option, but this PR obviously changes that. If we keep the PrettyConfig option as well, the extension would be used in non-pretty mode, but we’d need to decide how to merge the extension and the pretty config - or-ing them makes most sense. Either way, whether we keep the PrettyConfig option or not, I agree with you that we should add a short explanation to each item. |
Could you also bring back the coverage to 100%? There is a test in the error file where you can just add another case. Then, in your own test, you could either try to use asserts or manually disable coverage for the unreachable panics (search for GRCOV_EXCL_ in other tests for examples). All in all, thank you so much for working on this addition! Once these last minor nits are addressed, it’ll be ready to be merged. |
I believe I've fixed the formatting and coverage now. |
Ok, just a couple more minor nits - thank you again for implementing the extension! |
Yes, thank you! It seems one unrelated #[derive(Debug)] line has regressed in https://app.codecov.io/gh/ron-rs/ron/pull/522/blob/tests/non_string_tag.rs though |
Co-authored-by: Juniper Tyree <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@SylvKT Thank you so much again for implementing this extension! |
CHANGELOG.md
Fixes #519